Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reusable Transfer Screen #149

Merged
merged 3 commits into from
Dec 18, 2018
Merged

Reusable Transfer Screen #149

merged 3 commits into from
Dec 18, 2018

Conversation

SebastienGllmt
Copy link
Contributor

To enable transferring V2 wallets as discussed in #125
We will have two screens: One for v2 wallet transfer and one for Daedalus transfer

To make these screen easier to make, I extracted parts of the Daedalus transfer screen into customizable components that way we can reuse the UI code.

I also got rid of the dialog option on the transfer screen
image
Here is the rationale I gave for doing so

Is anybody opposed to removing the "no" option entirely and simplifying the dialog to JUST the button to "transfer all funds from Daedalus wallet"?
The `no` option is objectively a bad decision since it will be more tedious for them to create the transaction and it will generate a change address. Additionally, it seems like pretty obvious information
I want to make this page more generic to be reusable for paper wallets, yoroi import, etc. but it the option makes even less sense in that case (ex: "Do you have a hot wallet that already loaded your paper wallet? If so, go to the receive screen!" makes no sense)

Note: To avoid the PR getting too big, this PR only extracts out the components and doesn't actually implement the new v2 wallet transfer screen.

@SebastienGllmt SebastienGllmt added UI/UX Makes a visual change NFC No Functional Change (pure refactoring/cleanup) labels Dec 4, 2018
@nicarq
Copy link
Contributor

nicarq commented Dec 9, 2018

I will review it tomorrow

@nicarq nicarq self-requested a review December 14, 2018 01:28
vsubhuman
vsubhuman reviewed Dec 14, 2018
View reviewed changes
app/api/ada/daedalusTransfer.js
@@ -75,7 +75,7 @@ export async function generateTransferTx(payload: {
const recoveredBalance = await getBalance(senders);
const inputs = _getInputs(senderUtxos, addressesWithFunds);

// pick which address to send migration to
// pick which address to send transfer to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

nicarq
nicarq reviewed Dec 18, 2018
View reviewed changes
app/components/transfer/AnnotatedLoader.js
<div className={styles.title}>
{title}
</div>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this formatting is a little strange. is it manual?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(the line breaks)

@nicarq
Copy link
Contributor

nicarq commented Dec 18, 2018

LGTM

@nicarq nicarq merged commit 4f38603 into develop Dec 18, 2018
@nicarq nicarq deleted the feature/importYoroiWallet branch December 18, 2018 01:26
This was referenced Jan 14, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsubhuman vsubhuman vsubhuman left review comments

@nicarq nicarq nicarq approved these changes

Assignees
No one assigned
Labels
NFC No Functional Change (pure refactoring/cleanup) UI/UX Makes a visual change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SebastienGllmt @nicarq @vsubhuman